Conversation
| * type binding to an {@link UntypedNexusClientHandle} (returned by {@link | ||
| * NexusClient#getHandle(String)}) by calling one of the {@link #fromUntyped} factories. | ||
| */ | ||
| public interface NexusClientHandle<R> extends UntypedNexusClientHandle { |
There was a problem hiding this comment.
| public interface NexusClientHandle<R> extends UntypedNexusClientHandle { | |
| public interface NexusOperationHandle<R> extends UntypedNexusOperationHandle { |
Please keep consistent naming with other Handles like https://github.com/temporalio/sdk-java/blob/master/temporal-sdk/src/main/java/io/temporal/client/ActivityHandle.java
There was a problem hiding this comment.
The nexus doc had "NexusOperationHandle". I didn't use that name as there was already a NexusOperationHandle class and I didn't want to duplicate it. Even if in a different package, that just seemed confusing.
The reason I went to NexusClient in a lot of class names was to avoid naming collisions like that - again, even if in different packages it seemed confusing, and I wanted some form of consistency. So I named this NexusClientHandle to show that it was linked to the other NexusClient classes.
That being said, I do want to be consistent, but might have to check in with you and talk this one out. Maybe we can make these NexusOperationExecutionHandle and UntypedNexusOperationExecutionHandle, but then we lose the link to the other NexusClient classes - though would we use these for all Nexus operations so maybe we don't need such a link?
There was a problem hiding this comment.
Renamed to NexusOperationHandle as per conversation - we feel that the names being in different packages that have different use cases which should never be mixed should be sufficient to avoid confusion, especially as the user will get this returned to them and won't be creating these classes.
Leaving this conversation unresolved to make sure the SDK team sees it and can weigh in!
|
Reviewed most of the public API, didn't get to into the tests or implementation for now since some stuff will likely change. |
Quinn-With-Two-Ns
left a comment
Quinn-With-Two-Ns
left a comment
There was a problem hiding this comment.
Approving, assuming we address the cancel tests and any open SDK team discussions
| public <T> NexusServiceClient<T> newNexusServiceClient(Class<T> service, String endpoint) { | ||
| enforceNonWorkflowThread(); | ||
| return WorkflowThreadMarker.protectFromWorkflowThread( |
There was a problem hiding this comment.
newUntypedNexusServiceClient should call enforceNonWorkflowThread and protectFromWorkflowThread too.
| @Override | ||
| public ListNexusOperationExecutionsOutput listNexusOperationExecutions( | ||
| ListNexusOperationExecutionsInput input) { | ||
| // Pagination is an internal concern; the interceptor surface sees a single query in and a | ||
| // materialized list out. The loop bounds itself by the server-supplied next_page_token. | ||
| // stream of deserialized executions out. The loop bounds itself by the server-supplied | ||
| // next_page_token, accumulating all pages before streaming deserialized results. | ||
| java.util.List<io.temporal.api.nexus.v1.NexusOperationExecutionListInfo> all = | ||
| new java.util.ArrayList<>(); |
There was a problem hiding this comment.
The implementation should lazily fetch pages as needed. You can use EagerPaginator to implement it (see how RootActivityClientInvoker.listActivities and ListActivityExecutionIterator use it.)
4 participants
What was changed
Why?
Checklist
Closes
How was this tested: